-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enable build pods to tolerate running on crio while talking to docker #16491
enable build pods to tolerate running on crio while talking to docker #16491
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
15bee68
to
9eb991d
Compare
@mrunalp @openshift/devex ptal the s2i vendor changes will be replaced with a bump commit shortly. |
pkg/build/builder/util.go
Outdated
func readResolvConfHostPath() (string, error) { | ||
// find the /etc/resolv.conf host path based on what is mounted into this | ||
// container. | ||
resolvConf, err := os.Open("/proc/1/mountinfo") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to fail when we turn on shared pid namespace, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea. will containers no longer have their own /proc filesystem with their own pid 1 at that point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this won't work with pid namespace sharing. Cleaner way is using crio inspect endpoint but hopefully, we can move away from docker before we have to do that.
pkg/build/builder/util.go
Outdated
// readPid determines the actual host pid of the pid 1 process in this container | ||
func readPid() (string, error) { | ||
// get pid from /proc/1/sched , e.g.: "java (8151, #threads: 53)" | ||
pidFile, err := os.Open("/proc/1/sched") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow... that's hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and, let me emphasize, temporary until we move to pure crio support, however long that takes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaner way for getting pid as well as resolv path is from crio inspect endpoint but we didn't want that.. and yes this doesn't work with pid namespaces but hopefully we can migrate away from docker for builds by then or switch to using crio inspect endpoint.
pkg/build/builder/util.go
Outdated
// readResolveConfHostPath determines the path to the resolv.conf file for this | ||
// container, as it exists on the host machine. (/etc/resolv.conf is mounted | ||
// into the container from the host path). | ||
func readResolvConfHostPath() (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff needs to go into its own package. It's getting pretty ugly in here. These aren't build utils, they're build environment / os coupling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you really want to encourage other people to consume this stuff? again it should all go away in the future.
pkg/build/builder/docker.go
Outdated
@@ -305,7 +310,8 @@ func (d *DockerBuilder) dockerBuild(dir string, tag string, secrets []buildapi.S | |||
NoCache: noCache, | |||
Pull: forcePull, | |||
BuildArgs: buildArgs, | |||
NetworkMode: string(getDockerNetworkMode()), | |||
NetworkMode: network, | |||
BuildBinds: fmt.Sprintf("[\"%s:/etc/resolv.conf\"]", resolvConfHostPath), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I can inject : into the resolve conf host path, can I break this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'll break, i don't know that it'll get you anywhere.
if you can inject some quotes and a comma along with the colon you could presumably do pretty interesting things in terms of bindmounting host content into your build container.
I'm not sure how a user would have any control over that path though.
Regardless, we can certainly audit/strip those characters from the resolvConfHostPath.
pkg/build/builder/docker.go
Outdated
@@ -297,6 +297,11 @@ func (d *DockerBuilder) dockerBuild(dir string, tag string, secrets []buildapi.S | |||
return err | |||
} | |||
|
|||
network, resolvConfHostPath, err := getContainerNetworkMode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a lot better if this is an interface passed into docker builder, and none of this code had any idea of what this stuff was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what exactly are you proposing be an interface? A ContainerNetworkModeGetter interface?
1199dd6
to
57b6f4f
Compare
/retest |
1 similar comment
/retest |
57b6f4f
to
5e88801
Compare
/test crio |
7aa8ccd
to
902e5ec
Compare
/retest |
999ac00
to
c52a663
Compare
/test crio |
c43829a
to
c8f320a
Compare
@@ -178,6 +178,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e | |||
|
|||
setOwnerReference(pod, build) | |||
setupDockerSocket(pod) | |||
setupCrioSocket(pod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a check to add this only if the socket source path exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see a good way to do that(after all this logic is running in a controller that doesn't know anything about the target node for the build pod) and it doesn't do any harm to do the mount even if the source path doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the controller doesn't add the mount to the pod if it can't find the source? If the mount is passed down to the runc config.json, it will fail to start the container if it can't find the source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the controller adds the mount. if runc is the container engine, then the source will be there.
and docker seemingly does not care if it does not exist, because this is working when i run using docker as the container engine.
5d6d55c
to
33e17c1
Compare
33e17c1
to
57a4f0d
Compare
@mrunalp comments addressed |
LGTM |
/retest |
@bparees: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest |
Automatic merge from submit-queue |
I can't find any reference to |
Also, why did we expand the docker api? |
|
It does not. Not on regular docker.
…On Sun, Feb 25, 2018 at 3:32 PM, Ben Parees ***@***.***> wrote:
docker build -v provides this on the cli, I don't remember what the issue
was w/ the docker api itself not exposing it (or how docker build handles
it if the api doesn't provide it)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16491 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p5sPcab3cagZ9Ng6Prqu9E1xDMEmks5tYcNXgaJpZM4Pf0g_>
.
|
you mean "docker build -v" isn't even an option in regular docker? |
Correct
…On Sun, Feb 25, 2018 at 11:30 PM, Ben Parees ***@***.***> wrote:
It does not. Not on regular docker.
you mean "docker build -v" isn't even an option in regular docker?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16491 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p5jkqjvPgN6TcbdqE7iayJHvGRRNks5tYjN_gaJpZM4Pf0g_>
.
|
It's not indeed, we've added that long time ago @rhatdan |
guessing it got added as a "might as well" when the logic was being added to bindmount subscription secrets into the build container. |
Where's the implementation for this? I can't find it in projectatomic/docker for some reason. |
It's in branches 1.12.6, 1.13.1 |
I see ImageBuildOptions having "binds". It doesn't have "buildbinds".
…On Tue, Feb 27, 2018 at 3:37 AM, Antonio Murdaca ***@***.***> wrote:
It's in branches 1.12.6, 1.13.1
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16491 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pw-cxR3mbNRo0ThAqLDY1LgSRhx_ks5tY76ygaJpZM4Pf0g_>
.
|
"buildbinds" comes from the patched library, I think it's just the struct in the go client library, not in projectatomic/docker, That is translated to json as "buildbinds"
|
No description provided.